Give crq its own place on disk for a repository#53
Conversation
Everything crq does with git assumes it was RUN inside the checkout it cares about: the local-work probe shells out with no directory, target inference reads the current branch. That is right for a command an agent types from its working copy, and useless for the daemon, which has no checkout of any repository it reviews — the reason dispatch has nothing to stand on. Workspace separates the two: a bare mirror per repository, fetched rather than re-cloned, and a throwaway detached worktree per head. Detached on purpose — a place to inspect and build, not a branch to commit to by accident. A worktree left behind by a killed process is replaced rather than reused, since the head it holds is stale anyway. Credentials stay out of it. The remote is the ordinary https URL, so the host's existing git credential helper supplies the token and crq never holds a secret it would then have to avoid logging. One runner now executes every git command, taking the directory to run in, and it folds stderr into the error — "exit status 128" alone has never told anybody what went wrong. localWork takes that directory too, through Config.WorkDir, so a caller working in a worktree it made can say so without the Service being copied. The tests build a real repository and clone it, because the thing under test is whether the git invocations are right.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesWorkspace checkout lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Service
participant Workspace
participant Mirror
participant Checkout
Service->>Workspace: create configured workspace
Service->>Workspace: Checkout(repo, pr, sha)
Workspace->>Mirror: create or refresh bare mirror
Mirror-->>Workspace: mirror directory
Workspace->>Checkout: create detached worktree
Checkout-->>Service: checkout directory
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8361991202
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Six review findings on the workspace, two of them P1. The mirror tree was created with the umask's 0755, so on a shared host git wrote a private repository's objects and source world-readable under ~/.cache. The root is 0700 now, enforced rather than assumed. Clones were unauthenticated in the documented token-only setup: git does not read GITHUB_TOKEN or GH_TOKEN by itself, so a daemon with no credential helper would have failed at its first private checkout — dispatch's first real act. A credential helper is injected when crq has a token, and the token travels in the environment, never in argv, so a process listing, a log line and this package's own error strings carry the helper snippet and not the secret. Three ways a checkout could destroy another. Two workers first cloning one repository both passed the missing-HEAD check and cloned into the same directory; the clone now lands in a staging directory and is moved into place, and losing that race is fine because the winner's mirror is just as good. "a-b/c" and "a/b-c" joined with a dash are the same path, so one repository's cleanup deleted the other's live worktree; owner and name stay separate components. And a deferred Remove on a stale handle deleted the checkout that replaced it — each checkout now owns a generation directory and removes only its own. CRQ_WORKSPACE is read through Config, so a value in ~/.config/crq/env is actually used instead of being silently ignored by the daemon.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0506d379b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
✅ Action performedReview finished.
|
Four findings, one P1. Reading only GITHUB_TOKEN and GH_TOKEN meant the documented `gh auth login` setup produced unauthenticated clones: API calls worked, and every private checkout failed at dispatch's first real act. git now gets the same token the API client resolves, `gh auth token` included. A relative workspace root put the worktree somewhere other than where the returned path said. `git worktree add` runs inside the mirror, so the relative directory landed under the mirror while Checkout.Dir pointed at a path that did not exist. Roots are absolute now. Clearing a PR's directory before making a new generation force-removed a checkout another worker might be building in. Old generations are pruned by age instead, which collects what a killed process left without touching a live session. And two workers fetching one mirror race on git's ref locks, so the loser reported "cannot lock ref" although the winner had just made the mirror current. That retries briefly and then accepts the mirror as it stands, rather than failing a dispatch over a fetch somebody else finished.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bff9ecdf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Four findings, one P1 that would have destroyed a fix session's work. The mirror was a --mirror clone, whose refspec is +refs/*:refs/*, so the next `fetch --prune` reached into refs/heads and deleted any branch a session had created in its worktree — the documented way for a session to make changes. It is a bare clone now, fetching into refs/remotes/origin/*, which leaves refs/heads to the sessions. Pruning read the checkout directory's own timestamp, which editing files inside does not update: a session busy for twelve hours read as abandoned and had its worktree force-removed. It measures the newest file under the checkout instead. Persistent ref-lock contention on a shared mirror returned an error even though the mirror was current, failing a dispatch over another worker's success — the exact case concurrent dispatch has to survive. And the worktree add now goes through the credential-carrying runner, so a checkout filter that fetches from a private repository is authenticated like every other git call.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16c60043c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/crq/workspace.go`:
- Around line 306-330: Convert removeWorktree from a Workspace method to a
package-level function, since it does not use Workspace state. Update
Checkout.Remove to call the function directly instead of constructing
Workspace{}; preserve the existing removal and pruning behavior.
- Around line 199-244: Harden the git worktree creation in Workspace.Checkout by
adding a `--` argument separator before the positional dir and sha arguments in
the `w.git` call. Preserve the existing detached worktree behavior and error
handling while ensuring neither argument can be interpreted as a git option.
- Around line 363-377: The new Service.workspace and gitToken declarations are
unused. Wire Service.workspace into the dispatch flow so workspace construction
uses the configured root and resolved git token, ensuring gitToken is reached;
if dispatch integration is intentionally deferred, remove both declarations
instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b244612-27d6-4be8-b1d6-680b3fe32398
📒 Files selected for processing (6)
internal/crq/config.gointernal/crq/next.gointernal/crq/target.gointernal/crq/workspace.gointernal/crq/workspace_test.gointernal/gh/github.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
internal/gh/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep GitHub REST/GraphQL transport and the GitHub REST quota concept (
Throttle,ThrottleWait,IsThrottled) ininternal/gh; matching check runs to bots must use dialect classification rather than gh logic.
Files:
internal/gh/github.go
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep
internal/crqlimited to orchestration and effects wiring;service.gois the only effects executor for CAS writes andPostIssueComment, andDryRunmust write nothing.
Files:
internal/crq/config.gointernal/crq/next.gointernal/crq/target.gointernal/crq/workspace_test.gointernal/crq/workspace.go
internal/crq/config.go
📄 CodeRabbit inference engine (AGENTS.md)
Add a default entry in
parseCoBotswhen a new co-reviewer should be enabled by default.
Files:
internal/crq/config.go
🪛 golangci-lint (2.12.2)
internal/crq/workspace_test.go
[medium] 17-17: G301: Expect directory permissions to be 0750 or less
(gosec)
[medium] 31-31: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
internal/crq/workspace.go
[medium] 129-129: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 346-346: G204: Subprocess launched with variable
(gosec)
[error] 296-296: error is not nil (line 294) but it returns nil
(nilerr)
[error] 366-366: func (*Service).workspace is unused
(unused)
[error] 377-377: func gitToken is unused
(unused)
🔇 Additional comments (10)
internal/crq/workspace.go (5)
1-112: LGTM!
114-197: LGTM!
246-304: LGTM!
332-361: LGTM! (gosec's G204 here is expected — runninggitwith dynamic args is this file's whole purpose.)
379-389: LGTM!internal/crq/workspace_test.go (1)
1-321: LGTM!internal/crq/config.go (1)
50-59: LGTM!Also applies to: 184-184
internal/gh/github.go (1)
170-174: LGTM!internal/crq/next.go (1)
4-5: LGTM!Also applies to: 291-296, 315-324
internal/crq/target.go (1)
5-7: LGTM!Also applies to: 119-123
Applying the refspec only when cloning left every mirror made by an earlier crq still fetching +refs/*:refs/*. A fix session that created a branch in its worktree then wedged the WHOLE repository: git refuses to fetch into a branch checked out somewhere, so every later checkout of every PR failed with "refusing to fetch into branch ... checked out at". Observed live — one session's branch stopped the drain from dispatching anything for hours, while PRs sat with findings nobody was looking at. The refspec is now enforced on every Mirror call, which migrates the mirrors that already exist. The test reproduces the original failure: an old refspec, a branch created in a worktree, and a fetch for a different PR that has to keep working.
|
@coderabbitai review |
✅ Action performedReview finished.
|
A worktree is made for somebody else to work in, and that somebody runs a plain `git push`. Every git command in this package injects the credential helper with -c, which lasts exactly as long as one command, and git reads no GITHUB_TOKEN of its own — so on a host holding only a token, a caller could do all of its work in a checkout and fail at the last step of every one of them. Merging this as it stood would have taken that fix away from the drain again. So the mirror carries the helper SNIPPET in its own config, written on the clone path as well as the migration one: another worker picks a mirror up as soon as it is renamed into place, and would otherwise find one with nothing for its own commands to use. The secret is still not on disk — what is persisted reads TokenEnv from the environment, so a mirror somebody finds hands out nothing. TokenEnv is exported for the same reason: a caller running git in a checkout has to set it, which makes the name part of the contract rather than a detail. The read uses gitDir rather than w.git deliberately: w.git injects a helper of its own, so the check would answer with that injected value and conclude the mirror was configured when its config was empty.
|
@codex review |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89e76f5856
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7958262b9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0bf7f41e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f508166983
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…pace # Conflicts: # internal/crq/config.go
Roadmap item I, and the groundwork
crq watch --dispatch(item J) has been waiting on.The gap
Every git call in crq runs in the process's own working directory:
That is right for a command an agent types from its working copy. It is useless for the daemon,
which has no checkout of any repository it reviews — which is exactly why dispatch has nothing to
stand on. There are no clone, fetch, worktree or push calls anywhere in the repository today.
Workspace
A bare mirror per repository, fetched rather than re-cloned, and a throwaway worktree per head:
Three decisions worth stating:
Detached HEAD. This is a place to inspect and build, not a branch to commit to by accident. A
caller that means to write creates its own branch.
Mirrors are reused, worktrees are not. The expensive clone happens once per repository rather
than once per dispatch; a worktree left behind by a killed process is replaced, since the head it
holds is stale anyway.
Credentials are not crq's business. The remote is the ordinary https URL, so whatever credential
helper the host already has (
gh auth setup-gitwrites one) supplies the token. crq never holds asecret it would then have to avoid logging.
Also
One
gitDir(ctx, dir, args...)runs every git command now, and folds stderr into the error —exit status 128on its own has never told anybody what went wrong.localWorktakes a directorythrough
Config.WorkDir, so a caller working in a worktree it made can say so; that is a configfield rather than a copied Service, because Service carries rotation counters a copy would silently
fork.
Mirror/Checkoutrefuse a repo string that is not exactlyowner/name— those become pathsegments, and
owner/../../etcmust not be joined onto the workspace root.Verification
The tests build a real git repository and clone it, because what is under test is whether the git
invocations are right, not whether a mock returns what I told it to. They cover: clone then fetch
into the same mirror, a worktree with the committed content at the right SHA, detached HEAD,
re-checkout over a leftover worktree, removal, and removal of an already-removed worktree.
Path-escape names are refused, and the error-message test asserts git's own stderr survives.
gofmt -l .clean,go vet ./...clean,go test ./... -count=1green. Local preflight: 0 findings.Ref #42
Summary by CodeRabbit
New Features
Improvements